Skip to content

lint on unnecessary collections #15830

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

lint on unnecessary collections #15830

wants to merge 3 commits into from
+187 −19

Conversation

bend-n
Copy link
Contributor

@bend-n bend-n commented Oct 7, 2025

lints on

fn squares() -> Vec<(u8, u8)> {
 (0..8u8)
   .flat_map(|x| (0..8).map(move |y| (x, y)))
   .collect::<Vec<_>>()
}

changelog: [unnecessary_collect]: create lint

see also #clippy > linting on vector returns where return is collected

name can be bikeshed

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 7, 2025
@rustbot
Copy link
Collaborator

rustbot commented Oct 7, 2025

r? @llogiq

rustbot has assigned @llogiq.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 7, 2025 •
edited
Loading

Lintcheck changes for 98f1059

Lint Added Removed Changed
clippy::unnecessary_collect 36 0 0

This comment will be updated if you push new changes

@bend-n bend-n force-pushed the returning_vector_produced_from_from_iterator branch from 1b7f7a8 to a7a20c0 Compare October 7, 2025 13:23
@rustbot
Copy link
Collaborator

rustbot commented Oct 7, 2025

Some changes occurred in clippy_lints/src/doc

cc @notriddle

@bend-n bend-n force-pushed the returning_vector_produced_from_from_iterator branch 3 times, most recently from a8d01c4 to da2cf18 Compare October 7, 2025 13:41
samueltardieu
samueltardieu reviewed Oct 7, 2025
View reviewed changes
clippy_lints/src/doc/suspicious_doc_comments.rs Outdated
}
})
.collect()
fn collect_doc_replacements(attrs: &[Attribute]) -> impl Iterator<Item = (Span, String)> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why make it more complicated when the caller collects it because it needs a Vec? This should probably a restriction lint.

clippy_config/src/conf.rs
@@ -1,3 +1,4 @@
#![allow(clippy::unnecessary_collect)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use #[expect] to ignore a lint, and put it as close as possible to the lint emission.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bit difficult, its define_conf!

y21
y21 reviewed Oct 7, 2025
View reviewed changes
tests/ui/unnecessary_collect.stderr Outdated
Comment on lines 1 to 5
error: this collection may not be necessary. consider returning the iterator itself
--> tests/ui/unnecessary_collect.rs:5:12
|
LL | fn bad() -> Vec<u32> {
| -------- help: perhaps: `impl Iterator<Item = u32>`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest moving the "consider ..." part of the primary message into the help message to replace the "perhaps". IIRC at least rust-analyzer only shows the help message without the code suggestion in the quick fix tab, so just "help: perhaps" would look odd. It also makes the message itself more concise which is always nice

clippy_lints/src/unnecessary_collect.rs Outdated
/// ```
#[clippy::version = "1.92.0"]
pub UNNECESSARY_COLLECT,
pedantic,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this should be a restriction lint, at least as it's currently implemented because it seems too eager to me.

It's certainly useful in a lot of cases, but fundamentally I don't think there is anything wrong with this pattern. I get that it's for beginners, but clippy::pedantic is also enabled by experienced users, and this already introduces quite a few #[allow]s to the codebase.

There's also a bunch of false positives that seem fairly difficult to solve in a good way, like if the iterator has local lifetimes and can't be returned. Or if the method is part of a trait impl where the trait can't be controlled by the user in which case the signature can't be changed. The lint could be made weaker and checks could be added to avoid them, but maybe the goal is to strictly find all instances of this pattern without false negatives even if it means false positives, and you're willing to go through them manually and do some refactoring. If that's the case (which this sounds like to me), restriction sounds more fitting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ada4a was talking about how it would be good for pedantic..?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not like I'm a team member or anything though, just wanted to share my opinion^^

But yes, some of the linted cases being unsolvable is unfortunate, though pedantic lints are allowed to give more false-positives than the regular ones

@bend-n bend-n force-pushed the returning_vector_produced_from_from_iterator branch from da2cf18 to 98f1059 Compare October 7, 2025 14:04
@rustbot
Copy link
Collaborator

rustbot commented Oct 7, 2025

☔ The latest upstream changes (possibly 0a2eece) made this pull request unmergeable. Please resolve the merge conflicts.

@bend-n bend-n force-pushed the returning_vector_produced_from_from_iterator branch from 98f1059 to b359f33 Compare October 8, 2025 10:56
justDeeevin
justDeeevin reviewed Oct 10, 2025
View reviewed changes
clippy_lints/src/unnecessary_collect.rs
///
/// ### Why is this bad?
///
/// This might not be necessary, and its more idiomatic to simply return the iterator, giving the caller the option to use it as they see fit.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// This might not be necessary, and its more idiomatic to simply return the iterator, giving the caller the option to use it as they see fit.
/// This might not be necessary, and it's more idiomatic to simply return the iterator, giving the caller the option to use it as they see fit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samueltardieu samueltardieu samueltardieu left review comments

@y21 y21 y21 left review comments

+2 more reviewers

@ada4a ada4a ada4a left review comments

@justDeeevin justDeeevin justDeeevin left review comments

Reviewers whose approvals may not affect merge requirements
Assignees

@llogiq llogiq

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@bend-n @rustbot @samueltardieu @y21 @ada4a @justDeeevin @llogiq